[CLI-321] Add and use a Converter interface and implementations without using BeanUtils by Claudenw · Pull Request #216 · apache/commons-cli

Claudenw · 2024-01-03T10:51:22Z

https://issues.apache.org/jira/browse/CLI-321

Similar to #215 but without using BeanUtils.

Implementation introduces Converter and Verifier interfaces.

Converter does as it says and converts from Str to object.
Verifier verifies that string values are proper before doing the conversion. Checks are made when the string is added to the values in the Option object.

TypeHandler is reworked to use and manage the mapping of the Converter and Verifier instances to Classes.
Methods are provided to register Converter and Verifier for a class.

Tests added for Converters and Verifiers as well as TypeHandler changes.

Changes to earlier tests:

There was a TypeHandler test that expected the value "3,5" to throw and exception when it was retrieved, that now throws an exception when the value is initially added to the Option during command line parsing. Test has been updated to account for this.

Some javadocs need to be completed.

Adds BeanUtils dependency to POM and implements TypeHandler using the BeanUtils classes. Handles all methods that were in the original TypeHandler as well as other default classes provided by BeanUtils. Test updated to show proper parsing of types that previously were not implemented. Includes new cli.converters package that may be better handled by BeanUtils as it moves forward to support FunctionalInterfaces.

Added getParsedOptionValues methods with default values. Fixed som javadoc,

codecov-commenter · 2024-01-03T10:52:20Z

Codecov Report

❌ Patch coverage is 82.47423% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.01%. Comparing base (b3cdbea) to head (2d6ff50).
⚠️ Report is 574 commits behind head on master.

Files with missing lines	Patch %	Lines
.../main/java/org/apache/commons/cli/TypeHandler.java	76.59%	11 Missing ⚠️
.../main/java/org/apache/commons/cli/CommandLine.java	75.00%	3 Missing ⚠️
...in/java/org/apache/commons/cli/ParseException.java	71.42%	1 Missing and 1 partial ⚠️
...a/org/apache/commons/cli/PatternOptionBuilder.java	92.30%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #216      +/-   ##
============================================
- Coverage     93.20%   92.01%   -1.19%     
- Complexity      570      582      +12     
============================================
  Files            21       22       +1     
  Lines          1206     1253      +47     
  Branches        216      210       -6     
============================================
+ Hits           1124     1153      +29     
- Misses           46       63      +17     
- Partials         36       37       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

garydgregory · 2024-01-03T11:11:57Z

Hi @Claudenw
Thank you for the PR.
Run 'mvn' by itself before pushing to catch these build failures.

Claudenw · 2024-01-04T10:57:22Z

@garydgregory I think this might be ready for prime time now.

garydgregory

Hello @Claudenw
Thank you for this PR.
I think the best path is to work toward getting this PR in instead of the BeanUtils version.
Please see my comments, which are few, per my issue formatting.

garydgregory · 2024-01-05T12:32:12Z

+ * @since 1.7
+ */
+@FunctionalInterface
+public interface Verifier {


Probably more flexible if this extends Predicate<String>.

I think we can get rid of Verifier and just use Predicate

Note that the advantage of a custom interface here is that we are cementing the design to only test String input, as opposed CharSequence for example (or anything else). So we should make it clear at least in our mind what the intent is here or if there should be such an input-type restriction.

garydgregory · 2024-01-05T12:33:06Z

+/**
+ * The definition of the functional interface to call when verifying a string
+ * for input Like {@code Predicate<String>} but can throw a RuntimeException.
+ * @since 1.7


The next feature version will be 1.7.0 (major.minor.maintenance).

garydgregory · 2024-01-05T12:36:18Z

     *
-     * @param str the File location
-     * @return The file represented by {@code str}.
+     * @param      str the File location


Please revert the formatting changes. It makes the PR noisier, larger and takes longer to review.

garydgregory · 2024-01-05T12:37:27Z

+ * @since 1.7
+ */
+@FunctionalInterface
+public interface Converter<T> {


Probably should extend Function<String, T>.

It can't since Converter throws a ParseException and Function<String,T> does not allow that.

Right, we don't want to depend on Commons Lang's FailableException here.

Next: Throwing Exception is an anti-pattern and should be avoided at all costs IMO. Either:

reconsider and throw runtime exception OR

throw a more specific checked exception OR

parameterize the exception type

garydgregory · 2024-01-05T12:40:09Z

-            Integer.class);
-        checkOption(Option.builder().option("a").desc("desc").type(Integer.class).build(), "a", "desc", null, Option.UNINITIALIZED, null, false, false,
-                defaultSeparator, Integer.class);
+        checkOption(Option.builder("a").desc("desc").build(), "a", "desc", null, Option.UNINITIALIZED, null, false,


What's changed here? If it's just formatting, please revert, I don't want to take the time to tease apart formatting from feature changes.

garydgregory · 2024-01-05T12:48:00Z

How about adding support for Path?

Claudenw · 2024-01-05T13:21:42Z

@garydgregory Sorry about the formatting.

What would you want to see for Path?

I think that Verifier class should become a static class that simply contains static common Predicate instances.

Where Verifier is currently used in the code it will become Predicate .

I don't think there needs to be a difference. At one time I had the Verifier throwing the ParseException, but that has been fixed, so there is no valid distinction.

What are your thoughts?

I also want to move everything in o.a.c.cli.converters into o.a.c.cli

garydgregory · 2024-01-05T19:06:21Z

@garydgregory Sorry about the formatting.

What would you want to see for Path?

I think that Verifier class should become a static class that simply contains static common Predicate instances.

Where Verifier is currently used in the code it will become Predicate .

I don't think there needs to be a difference. At one time I had the Verifier throwing the ParseException, but that has been fixed, so there is no valid distinction.

What are your thoughts?

I also want to move everything in o.a.c.cli.converters into o.a.c.cli

Path: feature parity with File.

garydgregory · 2024-01-06T14:58:46Z

@garydgregory Sorry about the formatting.
What would you want to see for Path?
I think that Verifier class should become a static class that simply contains static common Predicate instances.
Where Verifier is currently used in the code it will become Predicate .
I don't think there needs to be a difference. At one time I had the Verifier throwing the ParseException, but that has been fixed, so there is no valid distinction.
What are your thoughts?
I also want to move everything in o.a.c.cli.converters into o.a.c.cli

Path: feature parity with File.

Which can be done later.

garydgregory · 2024-01-06T15:27:01Z

Hi @Claudenw

Hm, I think this PR is overly complicated.

Specifically, I do not think there should be a split between verifying and converting, there should not be a whole Verifier infrastructure IMO: The converters do their parsing, and in this PR, the syntaxes between verifying and parsing don't match anyway.

For a simple example, it is legal for an Integer to have a leading + character but the regular expression for integers doesn't allow it.

It's worse if I want to have a Double parameter since the RE for that is large and complex as documented in Double.valueOf(String) and it's certainly not what this PR does:

     *  final String Digits     = "(\\p{Digit}+)";
     *  final String HexDigits  = "(\\p{XDigit}+)";
     *  // an exponent is 'e' or 'E' followed by an optionally
     *  // signed decimal integer.
     *  final String Exp        = "[eE][+-]?"+Digits;
     *  final String fpRegex    =
     *      ("[\\x00-\\x20]*"+  // Optional leading "whitespace"
     *       "[+-]?(" + // Optional sign character
     *       "NaN|" +           // "NaN" string
     *       "Infinity|" +      // "Infinity" string
     *
     *       // A decimal floating-point string representing a finite positive
     *       // number without a leading sign has at most five basic pieces:
     *       // Digits . Digits ExponentPart FloatTypeSuffix
     *       //
     *       // Since this method allows integer-only strings as input
     *       // in addition to strings of floating-point literals, the
     *       // two sub-patterns below are simplifications of the grammar
     *       // productions from section 3.10.2 of
     *       // The Java Language Specification.
     *
     *       // Digits ._opt Digits_opt ExponentPart_opt FloatTypeSuffix_opt
     *       "((("+Digits+"(\\.)?("+Digits+"?)("+Exp+")?)|"+
     *
     *       // . Digits ExponentPart_opt FloatTypeSuffix_opt
     *       "(\\.("+Digits+")("+Exp+")?)|"+
     *
     *       // Hexadecimal strings
     *       "((" +
     *        // 0[xX] HexDigits ._opt BinaryExponent FloatTypeSuffix_opt
     *        "(0[xX]" + HexDigits + "(\\.)?)|" +
     *
     *        // 0[xX] HexDigits_opt . HexDigits BinaryExponent FloatTypeSuffix_opt
     *        "(0[xX]" + HexDigits + "?(\\.)" + HexDigits + ")" +
     *
     *        ")[pP][+-]?" + Digits + "))" +
     *       "[fFdD]?))" +
     *       "[\\x00-\\x20]*");// Optional trailing "whitespace"
     *
     *  if (Pattern.matches(fpRegex, myString))
     *      Double.valueOf(myString); // Will not throw NumberFormatException
     *  else {
     *      // Perform suitable alternative action
     *  }

All of that to say, that you don't have to worry about any of this if you let a converter do its usual parsing work without trying to short-circuit the process with an additional parsing step. IOW, don't parse input twice, we don't need verifiers.

Nits:

@param Javadoc tags should be listed first.
We don't need a new Java package: Let's keep it all in the original package.
Rebase on git master, we are now using the FinalParameters Checkstyle rule.

Verifier interface became static class to hold common Predicate<String> instances for verification. Moved EnumVerifier into Verifier as a static method. All references to Verifier as a data type chagned to Predicate<String>.

…age.

…or getOptionValue

Claudenw · 2024-01-22T13:01:48Z

@garydgregory I have added the Path to the standard converters handled by the TypeHandler and added documentation for the entire span of this change.

I also added a set of getOptionValue() methods that accept a Supplier to create the defaults.

garydgregory

I'll keep repeating it ;-) We don't need a "verifier". If later, this proves a requirement, we can discuss adding that concept. I still think that "verifying" is doomed to fail with both false positives and false negatives, on top of being redundant, since "verification" is done implicitly when parsing, for example by Double#parseDouble().

Claudenw · 2024-01-24T08:40:30Z

In working on Cassandra Stress test tool where we are trying to move to commons-cli there are cases where we want to quickly identify incorrect textual input without having to go through expensive conversion processes. This also gives an easy way to verify things like enum values and provide better error messages than if you just call enum.valueOf(str). Providing the option of the verifier means that often the converter is a simple call to an existing method.

I did remove the Verifier management code so we are not setting them by default and the TypeHandler does not set them either. Much like setting the type using the verifier is optional.

Claudenw · 2024-01-26T13:47:34Z

@garydgregory I removed the verifier code and documentation.

garydgregory

@Claudenw
Thank you for your updates. Please see my comments.

garydgregory · 2024-01-26T14:44:04Z

+     * Converts any exception except {@code UnsupportedOperationException} to a {@code ParseException}.
+     * if {@code e} is an instance of {@code ParseException} it is returned, otherwise a {@code ParseException} is 
+     * created that wraps it.
+     * <p>


Hello @Claudenw

Close HTML tags

Add @since tags to new public and protected elements.

garydgregory · 2024-01-26T14:44:21Z

+        if (e instanceof UnsupportedOperationException) {
+            throw (UnsupportedOperationException) e;
+        }
+


Remove extra white-space.

garydgregory · 2024-01-26T14:45:02Z

+
+    /**
+     * Registers custom {@code Converter}s with the {@code TypeHandler}. 
+     */


Add @since tags to new public and protected elements.

garydgregory · 2024-01-26T14:45:49Z

     *
     * @param ch the specified character
     * @return The class that {@code ch} represents
+     * @deprecated use {@link #getValueType(char)}


Add @Deprecated annotation to the method.

garydgregory · 2024-01-26T14:46:01Z

+     * @param ch the specified character
+     * @return The class that {@code ch} represents
+     */
+    public static Class<?> getValueType(final char ch) {


Add @since tags to new public and protected elements.

garydgregory · 2024-01-26T14:46:52Z

+    }
+
+    /**
+     * Registers a Converter for a Class.  If @code converter} is null registration is cleared for {@code clazz}, and 


Only one space after the ..

garydgregory · 2024-01-26T14:48:11Z

    System.out.println("Unexpected exception:" + exp.getMessage());
 }</source>
    </section>
+    <section name="Converting (Parsing) Option Values">


Nice! 😄 👏

Claudenw · 2024-01-29T09:54:43Z

@garydgregory requested changes made. Also found old 'verifier' documentation still in place and removed it.

garydgregory · 2024-01-29T14:52:20Z

+     */
+    public static void resetConverters() {
+        converterMap.clear();
+        converterMap.put(Object.class, Converter.OBJECT);


Hi @Claudenw,
We have a mix of using constants and expressions to define this map. I think it would be better to do it one way or another: Use all constants or use all expressions.
The simplest would be to inline the constants from Converter and decrease the new public footprint. Alternatively, to keep constants we could make them package-private in a new or existing class.
Any thoughts on this?

Claudenw added 4 commits December 29, 2023 14:02

Fixed issues with date parsing

7ae9b1c

Switched to ConvertUtilsBean2 for conversions.

ad8466c

Added getParsedOptionValues methods with default values. Fixed som javadoc,

Implementation with tests

e02c36c

Claudenw requested a review from garydgregory January 3, 2024 10:51

Claudenw marked this pull request as draft January 3, 2024 10:51

Claudenw mentioned this pull request Jan 3, 2024

Fixes for CLI-321 #215

Closed

Claudenw added 4 commits January 3, 2024 12:15

fixed checkstyle issues

9ae4c99

fixed spotbugs error & some javadoc

b5d2212

Updated spotbugs to 4.8.2.0

e36f282

added javadocs and tests

1787859

Claudenw self-assigned this Jan 4, 2024

Claudenw marked this pull request as ready for review January 4, 2024 10:57

Claudenw added 2 commits January 5, 2024 10:21

fixed breaking changes and javadoc

037550d

added since annotation to Converter and Verifier

e081e56

garydgregory requested changes Jan 5, 2024

View reviewed changes

Added an Enum Validator implementation

942364a

Claudenw added 2 commits January 5, 2024 14:12

fixed formatting issues

f365872

fixed checkstyle error

3ec932d

Claudenw and others added 3 commits January 11, 2024 11:45

Converted Verifier to Predicate<String>

6d5a916

Verifier interface became static class to hold common Predicate<String> instances for verification. Moved EnumVerifier into Verifier as a static method. All references to Verifier as a data type chagned to Predicate<String>.

Moved Converter, Verifier and their respective tests to base cli pack…

9b93936

…age.

Merge branch 'apache:master' into Functional_Converters

2ec9d4d

Claudenw added 2 commits January 11, 2024 13:26

Removed verifier management and rebased to master

a8d7c89

moved param <T> tags

d588a86

Claudenw requested a review from garydgregory January 15, 2024 11:38

Claudenw added 4 commits January 15, 2024 13:38

updated numberic tests

0eee294

Updated documentation, added Supplier<String> as a default provider f…

658aa2b

…or getOptionValue

fixed checkstyle issues

b68633a

Added Path to TypeHandler and updated documentation

1ca90bb

garydgregory requested changes Jan 22, 2024

View reviewed changes

Claudenw added 2 commits January 26, 2024 14:39

removed verifier

4d4f52c

removed verifier

30d26f5

garydgregory requested changes Jan 26, 2024

View reviewed changes

updated javadoc and site documentation

2d6ff50

garydgregory changed the title ~~Fixes for CLI-321 withoug using BeanUtils~~ Add and use a Converter interface and implementations without using BeanUtils Jan 29, 2024

garydgregory merged commit 9256a4d into apache:master Jan 29, 2024

garydgregory changed the title ~~Add and use a Converter interface and implementations without using BeanUtils~~ [CLI-321] Add and use a Converter interface and implementations without using BeanUtils Jan 29, 2024

garydgregory reviewed Jan 29, 2024

View reviewed changes

Claudenw deleted the Functional_Converters branch February 19, 2024 08:26

Uh oh!

Conversation

Claudenw commented Jan 3, 2024 • edited by garydgregory Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

garydgregory commented Jan 3, 2024

Uh oh!

Claudenw commented Jan 4, 2024

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory commented Jan 5, 2024

Uh oh!

Claudenw commented Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garydgregory commented Jan 5, 2024

Uh oh!

garydgregory commented Jan 6, 2024

Uh oh!

garydgregory commented Jan 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Claudenw commented Jan 22, 2024

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Claudenw commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Claudenw commented Jan 26, 2024

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Claudenw commented Jan 29, 2024

Uh oh!

Claudenw commented Jan 3, 2024 •

edited by garydgregory

Loading

codecov-commenter commented Jan 3, 2024 •

edited

Loading

garydgregory Jan 5, 2024 •

edited

Loading

garydgregory Jan 5, 2024 •

edited

Loading

Claudenw commented Jan 5, 2024 •

edited

Loading

garydgregory commented Jan 6, 2024 •

edited

Loading

Claudenw commented Jan 24, 2024 •

edited

Loading

garydgregory Jan 29, 2024 •

edited

Loading